Fix PHP 8.4 and 8.5 deprecations#229
Conversation
- Config\Repository: use '' rather than a null $namespace as the afterLoad array offset (PHP 8.5 'Using null as an array offset is deprecated'). - Halcyon\Processors\Processor: same for the fileName offset (array_get may return null). - Console\Traits\ProcessesQuery: explicit ?int $limit (PHP 8.4 implicit-nullable param). - Html\FormBuilder::selectMonth: replace deprecated strftime() with date() (default 'F'). - tests: update ScheduleListCommandTest invokable-class schedule:list expectation to match current Laravel 12.x rendering (pre-existing failure on wip/1.3, unrelated to the above).
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@austinderrick can you submit fixes to assetic/framework as well? We maintain that library. |
| // not want to load it again. Once items are loaded a first time they will | ||
| // stay kept in memory within this class and not loaded from disk again. | ||
| if (isset($this->afterLoad[$namespace])) { | ||
| if (isset($this->afterLoad[$namespace ?? ''])) { |
There was a problem hiding this comment.
How would namespace be null? Docblock currently only says string, what calls this method and passes it a null? Can we update this method to use real argument types?
| public function afterLoading($namespace, Closure $callback) | ||
| { | ||
| $this->afterLoad[$namespace] = $callback; | ||
| $this->afterLoad[$namespace ?? ''] = $callback; |
| protected function callAfterLoad($namespace, $group, $items) | ||
| { | ||
| $callback = $this->afterLoad[$namespace]; | ||
| $callback = $this->afterLoad[$namespace ?? '']; |
| $fileName = array_get($result, 'fileName'); | ||
|
|
||
| return [$fileName => $this->parseTemplateContent($query, $result, $fileName)]; | ||
| return [($fileName ?? '') => $this->parseTemplateContent($query, $result, $fileName)]; |
There was a problem hiding this comment.
Under what condition would this be null? That seems like a bigger issue and one that should be protected against by returning null earlier from this method.
| foreach ($results as $result) { | ||
| $fileName = array_get($result, 'fileName'); | ||
| $items[$fileName] = $this->parseTemplateContent($query, $result, $fileName); | ||
| $items[$fileName ?? ''] = $this->parseTemplateContent($query, $result, $fileName); |
There was a problem hiding this comment.
Under what condition would this be null? Seems like it would be a bigger issue if it did and would also cause issues with overwriting items if multiple returned null filename
| * Create a select month field. | ||
| */ | ||
| public function selectMonth(string $name, string|array|null $selected = null, array $options = [], $format = '%B'): string | ||
| public function selectMonth(string $name, string|array|null $selected = null, array $options = [], $format = 'F'): string |
There was a problem hiding this comment.
The Form facade would also need to be updated for this.
LukeTowers
added
the
needs response
2 participants
Summary
Fixes the PHP 8.4 and 8.5 deprecations.
Fixes
Using null as an array offset is deprecated, use an empty string instead):src/Config/Repository.php—afterLoad[$namespace](read/isset/write) now uses$namespace ?? ''(a null/global namespace is a valid case). (This is the one reported.)src/Halcyon/Processors/Processor.php—$items[$fileName]/[$fileName => …]now uses$fileName ?? ''(array_get($result, 'fileName')may return null).src/Console/Traits/ProcessesQuery.php—int $limit = null→?int $limit = null.strftime():src/Html/FormBuilder.php::selectMonth()— replacedstrftime()withdate(); the$formatdefault changes from'%B'to'F'(date() syntax). Default output (full month names) is unchanged; callers passing customstrftime-style formats must switch todate()syntax.tests/Scheduling/ScheduleListCommandTest— updated the invokable-classschedule:listexpectation (modern Laravel renders the bare class name without theClosure at:prefix). This was a pre-existing failure onwip/1.3unrelated to the deprecations.Testing
vendor/bin/phpuniton PHP 8.5.5: 711 tests, 0 failures, 0 Storm-source PHP deprecations.vendor/bin/phpuniton PHP 8.4.14: 711 tests, 0 failures, 0 Storm-source PHP deprecations.phpcs: clean on the changed files.Out of scope
vendor/assetic/frameworktriggers a$http_response_headerdeprecation on 8.5 — that's a third-party dependency, not Storm.wip/1.3(a larastan-version drift, in the relation generics / baseline) that are unrelated to this PR; they are addressed for the Laravel 13 line in Support Laravel 13 #228.